Code Quality: Improved WindowsStorables #17191

0x5bfa · 2025-06-14T10:43:34Z

Resolved / Related Issues

Code Quality: Replace Vanara with CsWin32 #15000

Steps used to test these changes

Launch the app
Load Home page

0x5bfa · 2025-06-14T17:55:22Z

src/Files.App.Storage/Storables/WindowsStorage/WindowsStorable.cs

+		public IShellItem* ThisPtr
+		{
+			[MethodImpl(MethodImplOptions.AggressiveInlining)]
+			get;
+
+			[MethodImpl(MethodImplOptions.AggressiveInlining)]
+			protected set;
+		}


@dongle-the-gadget Could you review for real quick? I unwrapped the ptr-s where they are passed to constructors of WindowsFile or WindowsFolder, which means they shouldnt be disposed outside the block where defined.

I just wanna make sure pointer managements around windows storables are done appropriately.

Do you have some code where the storables are used? In general you shouldn't call Release on the pointers you pass to the constructor as it looks like those objects take ownership.

I've ensured all of the instantiation is done witout releasing.

// Instead of: ComPtr<IShellItem> pShellItem = ...; var folder = new WindowsFolder(pShellItem.Get()); // Did this. because no reason to use ComPtr. IShellItem* pShellItem = ...; var folder = new WindowsFolder(pShellItem);

Files/src/Files.App.Storage/Storables/WindowsStorage/WindowsStorable.cs

Lines 36 to 55 in acac1a3

public static WindowsStorable? TryParse(string szPath)

{

HRESULT hr = default;

IShellItem* pShellItem = null;

fixed (char* pszPath = szPath)

hr = PInvoke.SHCreateItemFromParsingName(pszPath, null, IID.IID_IShellItem, (void**)&pShellItem);

if (pShellItem is null)

return null;

return TryParse(pShellItem);

}

public static WindowsStorable? TryParse(IShellItem* pShellItem)

{

bool isFolder = pShellItem->GetAttributes(SFGAO_FLAGS.SFGAO_FOLDER, out var returnedAttributes).Succeeded && returnedAttributes == SFGAO_FLAGS.SFGAO_FOLDER;

return isFolder ? new WindowsFolder(pShellItem) : new WindowsFile(pShellItem);

}

Files/src/Files.App/ViewModels/UserControls/Widgets/QuickAccessWidgetViewModel.cs

Lines 235 to 253 in acac1a3

unsafe

{

hr = PInvoke.RoGetAgileReference(AgileReferenceOptions.AGILEREFERENCE_DEFAULT, IID.IID_IShellItem, (IUnknown*)folderCardItem.Item.ThisPtr, pAgileReference.GetAddressOf());

}

// Unpin from Quick Access on Windows

hr = await STATask.Run(() =>

{

unsafe

{

IShellItem* pShellItem = null;

hr = pAgileReference.Get()->Resolve(IID.IID_IShellItem, (void**)&pShellItem);

using var windowsFile = new WindowsFile(pShellItem);

// NOTE: "unpinfromhome" is an undocumented verb, which calls an undocumented COM class, windows.storage.dll!CRemoveFromFrequentPlacesExecute : public IExecuteCommand, ...

// NOTE: "remove" is for some shell folders where the "unpinfromhome" may not work

return windowsFile.TryInvokeContextMenuVerbs(["unpinfromhome", "remove"], true);

}

});

Is everything then disposed?

Yes as of now.
But I'm very very concerned, this project is community driven; if someone uses these classes wrongly, the app immediately starts leaking memory/crashing out, etc. Being so, I probably am going to introduce a new analyzer to warn them at the instantiations that are done without using (if using should not be used at that point, they can suppress) but not clear how much that can help.

var folder = new WindowsFolder(pShellItem.Get()); ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ This class much be disposed of, consider to add 'using'.

Worth having it stand out:

#pragma disable warning FSG1xxx // ... var folder = new WindowsFolder(pShellItem.Get()); #pragma enable warning FGS1xxx // ...

yaira2 · 2025-06-15T02:26:16Z

Steps used to test these changes

Init

a9ef2a8

0x5bfa force-pushed the 5bfa/CQ-WindowsStorable branch from 671e280 to a9ef2a8 Compare June 14, 2025 17:49

0x5bfa marked this pull request as ready for review June 14, 2025 17:49

0x5bfa commented Jun 14, 2025

View reviewed changes

0x5bfa added 2 commits June 15, 2025 03:01

Improved

4b0fe97

Added ContextMenu and ShellNewMenu caches

acac1a3

yaira2 approved these changes Jun 15, 2025

View reviewed changes

yaira2 added the ready to merge Pull requests that are approved and ready to merge label Jun 15, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Code Quality: Improved WindowsStorables #17191

Code Quality: Improved WindowsStorables #17191

Uh oh!

0x5bfa commented Jun 14, 2025 •

edited

Loading

Uh oh!

0x5bfa Jun 14, 2025

Uh oh!

0x5bfa Jun 14, 2025

Uh oh!

dongle-the-gadget Jun 15, 2025

Uh oh!

0x5bfa Jun 15, 2025

Uh oh!

dongle-the-gadget Jun 15, 2025

Uh oh!

dongle-the-gadget Jun 15, 2025

Uh oh!

0x5bfa Jun 17, 2025 •

edited

Loading

Uh oh!

yaira2 commented Jun 15, 2025

Uh oh!

Uh oh!

	public static WindowsStorable? TryParse(string szPath)
	{
	HRESULT hr = default;
	IShellItem* pShellItem = null;

	fixed (char* pszPath = szPath)
	hr = PInvoke.SHCreateItemFromParsingName(pszPath, null, IID.IID_IShellItem, (void**)&pShellItem);

	if (pShellItem is null)
	return null;

	return TryParse(pShellItem);
	}

	public static WindowsStorable? TryParse(IShellItem* pShellItem)
	{
	bool isFolder = pShellItem->GetAttributes(SFGAO_FLAGS.SFGAO_FOLDER, out var returnedAttributes).Succeeded && returnedAttributes == SFGAO_FLAGS.SFGAO_FOLDER;

	return isFolder ? new WindowsFolder(pShellItem) : new WindowsFile(pShellItem);
	}

	unsafe
	{
	hr = PInvoke.RoGetAgileReference(AgileReferenceOptions.AGILEREFERENCE_DEFAULT, IID.IID_IShellItem, (IUnknown*)folderCardItem.Item.ThisPtr, pAgileReference.GetAddressOf());
	}

	// Unpin from Quick Access on Windows
	hr = await STATask.Run(() =>
	{
	unsafe
	{
	IShellItem* pShellItem = null;
	hr = pAgileReference.Get()->Resolve(IID.IID_IShellItem, (void**)&pShellItem);
	using var windowsFile = new WindowsFile(pShellItem);

	// NOTE: "unpinfromhome" is an undocumented verb, which calls an undocumented COM class, windows.storage.dll!CRemoveFromFrequentPlacesExecute : public IExecuteCommand, ...
	// NOTE: "remove" is for some shell folders where the "unpinfromhome" may not work
	return windowsFile.TryInvokeContextMenuVerbs(["unpinfromhome", "remove"], true);
	}
	});

Uh oh!

Code Quality: Improved WindowsStorables #17191

Are you sure you want to change the base?

Code Quality: Improved WindowsStorables #17191

Uh oh!

Conversation

0x5bfa commented Jun 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Resolved / Related Issues

Steps used to test these changes

Uh oh!

0x5bfa Jun 14, 2025

Choose a reason for hiding this comment

Uh oh!

0x5bfa Jun 14, 2025

Choose a reason for hiding this comment

Uh oh!

dongle-the-gadget Jun 15, 2025

Choose a reason for hiding this comment

Uh oh!

0x5bfa Jun 15, 2025

Choose a reason for hiding this comment

Uh oh!

dongle-the-gadget Jun 15, 2025

Choose a reason for hiding this comment

Uh oh!

dongle-the-gadget Jun 15, 2025

Choose a reason for hiding this comment

Uh oh!

0x5bfa Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yaira2 commented Jun 15, 2025

Uh oh!

Uh oh!

0x5bfa commented Jun 14, 2025 •

edited

Loading

0x5bfa Jun 17, 2025 •

edited

Loading